-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lw 9170 multi wallet integration #892
Conversation
Allure report
smokeTests: ✅ test report for bd11e2ef
|
...browser-extension-wallet/src/components/MainMenu/DropdownMenuOverlay/components/UserInfo.tsx
Show resolved
Hide resolved
...browser-extension-wallet/src/components/MainMenu/DropdownMenuOverlay/components/UserInfo.tsx
Outdated
Show resolved
Hide resolved
...r-extension-wallet/src/components/MainMenu/DropdownMenuOverlay/components/WalletAccounts.tsx
Outdated
Show resolved
Hide resolved
...r-extension-wallet/src/components/MainMenu/DropdownMenuOverlay/components/WalletAccounts.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outstanding work 🎉 love the descriptive and clean commits + it's working smoothly with multiple wallets and accounts!
I left some code suggestions / questions
Changes unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only found one issue related to hooks, everything else looks good. I need to run some tests locally
...s/ui/src/design-system/profile-dropdown/accounts/profile-dropdown-account-item.component.tsx
Show resolved
Hide resolved
...r-extension-wallet/src/components/MainMenu/DropdownMenuOverlay/components/WalletAccounts.tsx
Outdated
Show resolved
Hide resolved
@mkazlauskas thanks for the updates, all green from my side 🚢 |
9337d2f
to
c2d6a83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkazlauskas approved!
...s/ui/src/design-system/profile-dropdown/accounts/profile-dropdown-account-item.component.tsx
Show resolved
Hide resolved
282e3ce
to
7a410ef
Compare
Please do not merge https://input-output.atlassian.net/browse/LW-9170?focusedCommentId=260303 |
…t flow if create wallet flow finishes without explicitly calling setBackgroundPage() then it would break the routing, as background page would be defined forever
temporarily use native prompt dialog for password input; does not have a prompt to connect hardware wallet, or any error handling when deriving xpub
If it takes more than some time (probably 50ms), Chrome will reject hardware device connection with an error that says WebHID connections must be initiated from user gesture. Apparently, fetching wallets from repository can sometimes take longer. This fix updates useWalletManager addAccount method to take in the entire AnyBip32Wallet object instead of just walletId, so that it doesn't have to fetch the wallet object from indexeddb in service worker. Consequently, some validations from addAccount can be removed, because it's signature no longer accepts walletId of a script wallet. Also it has to trust that wallet actually exists, otherwise it will reject with an error that comes from the WalletRepository (it is no longer a responsibility of this method to check the existence of the wallet).
clicking on a wallet in dropdown menu activates it removing copy feature makes the behavior more predictable
show tooltip that explains the behavior
also decouple button color scheme from size scheme and set minWidth for ExtraSmall button, which is currently only used for profile dropdown account item
temporary design until we apply a larger re-design
toast is a temporary solution, to be replaced
onboarding, adding wallet, adding accounts and dapp transacations are now working
required for correctly bundling trezor
after removing imports from dist/cjs, service worker no longer loads this is a temporary solution
daa2543
to
e4ed066
Compare
Rebased and fixed "abs / relative paths" conflicts ( kept changes Martynas introduced ). |
@pczeglik-iohk I Checked 5b2832a Approved! 💯 🚀 |
apps/browser-extension-wallet/src/hooks/__tests__/useWalletManager.test.tsx
Outdated
Show resolved
Hide resolved
5b2832a
to
bd11e2e
Compare
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Checklist
Proposed solution
Integrate multi-wallet/account UI components with SDK utilities
Out of scope: enter password/connect hardware modal when enabling accounts (LW-9709)
Testing
Requires
USE_MULTI_WALLET=true
Screenshots